Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDBELGA-759] Feature: Sync Event metadata to Planning & Coverages #1892

Merged
merged 13 commits into from
Jan 8, 2024

Conversation

MarkLark86
Copy link
Collaborator

Also moves the EmbeddedPlanning from a front-end feature to be a back-end feature (so that metadata sync and associated planning can be handled with the 1 request, and the 1 DB change).

@MarkLark86 MarkLark86 added this to the 2.8 milestone Dec 22, 2023
@MarkLark86 MarkLark86 requested a review from petrjasek December 22, 2023 05:38
@MarkLark86
Copy link
Collaborator Author

@petrjasek The code is ready to be reviewed. I have placed this PR in draft state while I add tests

@MarkLark86 MarkLark86 marked this pull request as ready for review December 28, 2023 06:41
Copy link
Member

@petrjasek petrjasek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, what's your take on using dataclasses vs typed dicts, should we use both, only one, or have some kind of guidelines?

@MarkLark86
Copy link
Collaborator Author

MarkLark86 commented Jan 4, 2024

looks good, what's your take on using dataclasses vs typed dicts, should we use both, only one, or have some kind of guidelines?

I don't mind the use of dataclasses, as it provides:

  • both compile and runtime type checking
  • dot notation (which I find easier to read vs bracket notation).
  • strict typing (for cases where you know the exact structure of the data)

On the other hand, dataclasses should never be used with data that will be used in a:

  • response to a request from the API
  • request for a job/task through celery
  • request for data to be stored in the DB

This is because the data would have to be serialised, which means converting it to a dict first, in which case a dict should be used.

Thoughts @petrjasek?

@petrjasek
Copy link
Member

makes sense

@MarkLark86 MarkLark86 merged commit b6663cd into superdesk:develop Jan 8, 2024
17 checks passed
@MarkLark86 MarkLark86 modified the milestones: 2.8, 2.7 Jan 30, 2024
MarkLark86 added a commit to MarkLark86/superdesk-planning that referenced this pull request Jan 30, 2024
…uperdesk#1892)

* Config: Add new config for Event to Planning sync

* ui: Send `embedded_planning` from front-end

* api: Improve Planning ContentProfiles module and types

* api: Process `embedded_planning` and sync metadata

* fix(ui): Failed to get lock for recurring series

* fix(ui): Improve EmbeddedCoverageForm UI

* fix(ui): Event to Planning translations copies fields multiple times

* fix(e2e): Use chrome in CI to run tests

* Add behave tests for embedded planning

* Add behave tests for multilingual embedded planning

* Add CVs to behave test

* fix: Coverage Planning updated when processing Assignment details
Embedded/Sync wasn't working once above bug was fixed

* Add behave tests for event metadata sync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants